Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

migrate bbolt info command to cobra style #636

Closed
wants to merge 1 commit into from
Closed

migrate bbolt info command to cobra style #636

wants to merge 1 commit into from

Conversation

charles-chenzz
Copy link
Contributor

#472
migrate bbolt info to cobra style

jmhbnz
jmhbnz reviewed Dec 16, 2023
View reviewed changes
Copy link
Member

@jmhbnz jmhbnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @charles-chenzz - Thanks for your work on this. Please resolve test failures detected by make fmt.

@charles-chenzz charles-chenzz force-pushed the migrate_info_cmd branch 2 times, most recently from 96c8323 to 7cdde7a Compare December 16, 2023 08:51
@charles-chenzz
Copy link
Contributor Author

charles-chenzz commented Dec 16, 2023 •
edited
Loading

@jmhbnz sorry for the ping, but I've run make fmt and fix the import issue, the CI logs still failed

please ignore, I'll take a look on it

jmhbnz
jmhbnz suggested changes Dec 16, 2023
View reviewed changes
Copy link
Member

@jmhbnz jmhbnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing the fmt errors @charles-chenzz. I've reviewed the changes, please see two suggestions below.

cmd/bbolt/command_root.go Outdated Show resolved Hide resolved
cmd/bbolt/command_info.go
infoCmd := &cobra.Command{
Use: "info PATH",
Short: "Info prints basic information about the Bolt database at PATH.",
RunE: func(cmd *cobra.Command, args []string) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest validating args cobra style, if I don't supply the DB path currently and just run bbolt info all I get is a panic and it's unclear from a user perspective what was done wrong.

Example arg validation:

bbolt/cmd/bbolt/command_surgery.go

Lines 57 to 65 in cd4a757

Args: func(cmd *cobra.Command, args []string) error {
if len(args) == 0 {
return errors.New("db file path not provided")
}
if len(args) > 1 {
return errors.New("too many arguments")
}
return nil
},

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can use

Args: cobra.ExactArgs(1)

but I've use the example that you list

jmhbnz
jmhbnz approved these changes Dec 17, 2023
View reviewed changes
Copy link
Member

@jmhbnz jmhbnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - Thanks @charles-chenzz

@jmhbnz
Copy link
Member

jmhbnz commented Jan 4, 2024

Discussed during sig-etcd triage meeting. This would be a breaking change, we will hold merging this until we release 1.4.0.

@charles-chenzz
Copy link
Contributor Author

Discussed during sig-etcd triage meeting. This would be a breaking change, we will hold merging this until we release 1.4.0.

it's ok. Thanks for clarifying this! @jmhbnz

@github-actions github-actions bot added the stale label Apr 16, 2024
@github-actions github-actions bot closed this Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmhbnz jmhbnz jmhbnz approved these changes

Assignees
No one assigned
Labels
ok-to-test stale
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@charles-chenzz @jmhbnz